fix(shared): consistently remove boolean attributes for falsy values #4348
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TLDR; Boolean attributes do not handle falsy values consistently. The same value does not always lead to the same result, depending on other factors. This only impacts certain edge cases involving
0
,NaN
and''
. More common values, such asfalse
,null
andundefined
, are not affected.The origin of this PR is an issue we received in the docs repo: vuejs/docs#1173. I believe this is a bug rather than a documentation problem.
For most boolean attributes (e.g.
checked
,disabled
), the attribute is set via its corresponding property. An empty string is handled specially and is treated astrue
. Otherwise, usual JS truthy/falsy rules apply.However, some boolean attributes need to be handled specially because their property names aren't in lower-case. e.g.
readonly
/readOnly
. These attributes go through a totally different code path. In most cases they are handled consistently withchecked
anddisabled
. But for the edge cases involving some falsy values, such as0
andNaN
, it treats the value as truthy and includes the attribute.While
NaN
is a bit obscure, I can easily imagine0
being used as a value forreadonly
, e.g. as a result of checking the length of an array. e.g.<input :readonly="errors.length">
. Whatever 'correct' is, it should be the same for bothdisabled
andreadonly
.I think there are 3 options here to resolve the inconsistency:
null
,undefined
andfalse
as falsy values for the purposes of boolean attributes.disabled
currently behaves and only handle''
specially.In this PR I have gone with option 3. That fits with my own expectations about how it should behave and it also avoids any external change to how most boolean attributes behave at runtime.
As part of trying to implement this, I also looked into how it was handled in SSR. This involves considering a variety of cases depending on how an attribute is set. I found that SSR contained some further inconsistencies. For example, in some cases an empty string would not be considered 'true' from the perspective of a boolean attribute. I did some experiments with VitePress and found that
<input :disabled="d">
withd = ref('')
would give inconsistent results between dev and production builds. A production build would go throughcompiler-ssr
, leading to the attribute being omitted.This PR should also fix the problems with SSR, making it consistent with how boolean attributes are handled at runtime.
I've introduced a shared helper called
includeBooleanAttr
. The logic it contains is trivial but I think its real value is in tying together the various places in the code that duplicate the handling of boolean attributes. Using it inpackages/runtime-dom/src/modules/props.ts
isn't strictly necessary as the previous logic was already correct and, in practice, most code paths don't go through the helper. Nevertheless, I felt including it there was valuable from a maintenance perspective, as it's now possible to find everywhere that handles boolean attributes by searching for uses of that function.Some of the changes to the SSR specs look a bit weird because it's wrapping
_ssrLooseEqual
in_ssrIncludeBooleanAttr
. That isn't really necessary as_ssrLooseEqual
returns a boolean anyway but I didn't see an easy way to avoid it whilst still fixing the problem for other cases.